-
-
Notifications
You must be signed in to change notification settings - Fork 2k
AudioWorklet driver, misc. emscripten fixes #17750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Just bumped the default audio latency for AudioWorklet back down to 64 ms, though rwebaudio and OpenAL still need the higher latency in emscripten. BTW - for building RetroArch with AudioWorklet, use these extra variables: |
Makefile.emscripten
Outdated
| $(Q)$(LD) -o $@ $(RARCH_OBJ) $(libretro_new) $(LIBS) $(LDFLAGS) | ||
| @if [ "$(HAVE_AUDIOWORKLET)" = 1 ]; then\ | ||
| tr -d '\n' < "$${TARGET%%\.js}.aw.js" | sed -e "s/[\/&]/\\\\&/g" -e "s/'/\\\\\\\\&/g" > _audioworklet.js;\ | ||
| sed -i "s/\"$${TARGET%%\.js}\.aw\.js\"/URL.createObjectURL(new Blob(['$$(cat _audioworklet.js)'],{type:'text\/javascript'}))/" "$@";\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sed and make both have some differences on Mac OS here, so I was able to get it compiling on Mac and Linux with this variation of the $(TARGET) rule (there is a little duplication unfortunately).
I also tweaked the $(libretro_new) rule to only do the mv if the file with the wrong suffix exists. mv -f ... || true would be another valid choice there.
diff --git a/Makefile.emscripten b/Makefile.emscripten
index 83db5df9b0..36aeb6306c 100644
--- a/Makefile.emscripten
+++ b/Makefile.emscripten
@@ -293,18 +293,23 @@ RARCH_OBJ := $(addprefix $(OBJDIR)/,$(OBJ))
all: $(TARGET)
-$(libretro_new) : $(libretro)
+$(libretro_new) : $(wildcard libretro)
mv -f $(libretro) $(libretro_new)
# until emscripten adds something like WASM_WORKERS=2 but for audio worklets, DIY.
+ifeq ($(HAVE_AUDIOWORKLET), 1)
+TARGET_BASE := $(LIBRETRO)_libretro
+$(TARGET): $(RARCH_OBJ) $(libretro_new)
+ @$(if $(Q), $(shell echo echo "LD $@ \<obj\> $(libretro_new) $(LIBS) $(LDFLAGS)"),)
+ $(Q)$(LD) -o $@ $(RARCH_OBJ) $(libretro_new) $(LIBS) $(LDFLAGS)
+ $(Q)tr -d '\n' < "$(TARGET_BASE).aw.js" | sed -e "s/[\/&]/\\\\&/g" -e "s/'/\\\\\\\\&/g" > _audioworklet.js
+ $(Q)sed -i.bak -e "s/\"$(TARGET_BASE)\.aw\.js\"/URL.createObjectURL(new Blob(['$$(cat _audioworklet.js)'],{type:'text\/javascript'}))/" -- "$@"
+ $(Q)rm -f "$(TARGET_BASE).aw.js" _audioworklet.js "$@".bak
+else
$(TARGET): $(RARCH_OBJ) $(libretro_new)
@$(if $(Q), $(shell echo echo "LD $@ \<obj\> $(libretro_new) $(LIBS) $(LDFLAGS)"),)
$(Q)$(LD) -o $@ $(RARCH_OBJ) $(libretro_new) $(LIBS) $(LDFLAGS)
- @if [ "$(HAVE_AUDIOWORKLET)" = 1 ]; then\
- tr -d '\n' < "$${TARGET%%\.js}.aw.js" | sed -e "s/[\/&]/\\\\&/g" -e "s/'/\\\\\\\\&/g" > _audioworklet.js;\
- sed -i "s/\"$${TARGET%%\.js}\.aw\.js\"/URL.createObjectURL(new Blob(['$$(cat _audioworklet.js)'],{type:'text\/javascript'}))/" "$@";\
- rm "$${TARGET%%\.js}.aw.js" _audioworklet.js;\
- fi
+endif
$(OBJDIR)/%.o: %.c
@mkdir -p $(dir $@)| #elif defined(__wiiu__) | ||
| #include "../audio/drivers/wiiu_audio.c" | ||
| #elif defined(EMSCRIPTEN) | ||
| #elif defined(HAVE_RWEBAUDIO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add the audioworklet driver in here somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why rwebaudio is in griffin in the first place, maybe emscripten used to build using griffin? It probably doesn't matter, but I fixed this condition just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I like these improvements to my sketch for cross platform sed. I’ll review the code changes once I do some testing tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, I'm very happy to see this. Seems to work great in Safari on Mac and on iOS.
|
There are still a couple more (minor and iOS safari specific) issues that I would like to resolve before this is merged. |
|
That should be it! We're thinking the other bug - occasionally freezing - was due to using an out of date emscripten version (I was unable to reproduce it.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice job
| emscripten_thread_sleep(msec); | ||
| } | ||
| #elif defined(EMSCRIPTEN) | ||
| #define retro_sleep(msec) (emscripten_thread_sleep(msec)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you also check to make sure this was at least compiled with threads? Or are we assuming asyncify or threads is always true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above condition, which will be preferred if it isn't compiled with threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - emscripten_thread_sleep also works on the main thread, but it busy spins so it just locks the page. It isn't used in that case anyway.
|
Is it alright if I merge this? Or does work still need to be done? |
|
Yep, this is ready! |
Changes:
HAVE_EXTRA_WASMFS=1to enable OPFS/FetchFS now;HAVE_WASMFS=1will only enable WasmFS and not the extra FS backends.platform_emscripten.hsince we have several functions used from there nowBTW - regarding #15479 (comment):
This seems to be a firefox windows issue. Even luser.github.io/gamepadtest shows the same behavior.
Let me know what you think! Also, this still needs to be tested on safari.